-
Couldn't load subscription status.
- Fork 15k
[libc++] Add move constructor & assignment to exception_ptr
#164281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[libc++] Add move constructor & assignment to exception_ptr
#164281
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
fef6eee to
b595542
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we instead add a swap and implement operator= as swap(exception_ptr())? That would avoid having to introduce any new symbols, at least in this patch.
b595542 to
154c286
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM, just some nits.
|
I updated this commit to use implement the move-assignment using Is that what you had in mind? Benchmark results (original implementation) Benchmark results (swap-based implementation) Interpretation of benchmark results I consider the In particular The As soon as we inline the destructor, this regression should disappear again. As such, I think we can live with that temporary regression - WDYT? |
Yes, exactly.
Yeah, I think that's fine. Also, please update the commit message to mention that we introduce |
👍 then I think we have high-level alignment on this PR. The next step will be to polish it for final review.
I will do so after #164278 shipped, so I won't have to repeatedly rebase this PR and redo the measurements anymore One more question: |
Yeah, that should fix the CI. |
This commit adds move constructor and move assignment to
`exception_ptr`. Adding those operators allows us to avoid unnecessary
calls to `__cxa_{inc,dec}rement_refcount`.
Performance results:
```
Benchmark Baseline Candidate Difference % Difference
------------------------------- ---------- ----------- ------------ --------------
bm_nonnull_exception_ptr 52.22 40.92 -11.31 -21.65
bm_null_exception_ptr 31.41 23.29 -8.12 -25.85
bm_optimized_null_exception_ptr 28.69 20.50 -8.19 -28.55
```
This commit does not add a `swap` specialization. Thanks to the added
move-assignment, we already save a couple of increments/decrements also
in the default `swap` implementation. The default `swap` is still not
perfect, as it calls the desctructor on `tmp`. As soon as we also
inlined the `~exception_ptr` destructor fast-path for `__ptr ==
nullptr`, the optimizer should be able to optimize the default `swap`
just as well as a specialized `swap`, though.
154c286 to
0fcaeee
Compare
0fcaeee to
b106dc9
Compare
|
/libcxx-bot benchmark libcxx/test/benchmarks/exception_ptr.bench.cpp Benchmark results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some basic tests for the new functions? I'm pretty sure everything should be testable portably to some extent.
Co-authored-by: Nikolas Klauser <[email protected]>
This commit adds move constructor and move assignment and
swapto
exception_ptr. Adding those operators allows us to avoidunnecessary calls to
__cxa_{inc,dec}rement_refcount.Performance results: